Clamp teleportation position #4203
Conversation
|
My feeling here would be that simply rejecting a teleport request would be better than clamping the coordinates. I think adding a check in SetPosition as well would be a good idea - just clamping a float is not really the most intensive operation. |
src/Entities/Player.cpp
Outdated
| // This is necessary to avoid rounding errors in the noise generator and overflows in the chunk loader | ||
| const double ClampedPosX = Clamp(a_PosX, -167770000.0, 167770000.0); | ||
| const double ClampedPosY = Clamp(a_PosY, -167770000.0, 167770000.0); | ||
| const double ClampedPosZ = Clamp(a_PosZ, -167770000.0, 167770000.0); |
There was a problem hiding this comment.
I want at least a constant for these numbers.
If you could represent them with std::numeric_limits, it would be even better 😄 .
P.S.: Maybe also just update the a_Pos(X|Y|Z) variables to prevent anyone mixing the clamped and non clamped versions.
src/Entities/Player.cpp
Outdated
| // Clamp the positions to exactly representable single-precision floating point values | ||
| // This is necessary to avoid rounding errors in the noise generator and overflows in the chunk loader | ||
| const double ClampedPosX = Clamp(a_PosX, -167770000.0, 167770000.0); | ||
| const double ClampedPosY = Clamp(a_PosY, -167770000.0, 167770000.0); |
There was a problem hiding this comment.
This doesn't make that much sense.
You could for example teleport to y = -167770000 and then the falling mechanics should get us into bad numbers.
Clamp it to 0 - MAX_HEIGHT makes a lot more sense.
See The Rules of Code Optimization. Otherwise you could just add an |
A large position value would lead to a rounding error in the noise generator which uses float instead of double. A position greater than 0x08000000 * 16 would also lead to a failed assertion in
cWorldStorage::QueueLoadChunk. The clamping could be done incEntity:SetPositionfor extra safety, but that might impact performance. This fixes #4077